Refactor FastMCP to inherit from Provider#2901
Conversation
FastMCP now properly inherits from Provider, eliminating ~200 lines of duplicated _source_* methods. Key changes: - get_tool/resource/prompt return None instead of raising NotFoundError - Visibility filter separated from transforms (applied last) - Nested server middleware runs on both list and execution operations - Resource auth failure doesn't fall back to templates - AggregateProvider kept as user-facing utility class
Resolved conflicts to combine Provider inheritance refactor with versioning feature: - FastMCP now properly inherits from Provider - get_tool/resource/template/prompt return None instead of raising NotFoundError - Deduplication uses version_sort_key to keep highest version per name/URI - _source_* methods eliminated in favor of inherited _* methods
- Override _get_tool/resource/template/prompt in FastMCP to apply server transforms over provider aggregation - Update FastMCPProvider get_* methods to check for None (not NotFoundError since get_* now returns None) - Update versioning tests to expect None instead of NotFoundError when requesting filtered/nonexistent versions
- Add None checks in auth and tool transform tests - Add assertions in component_service.py for None returns - Add type ignore comments for max() with version_sort_key
…eritance-v2 # Conflicts: # src/fastmcp/server/server.py
Replace _get_all_transforms() with a .transforms property that returns [*self._transforms, self._visibility]. This cleanly separates user transforms from visibility filtering while keeping visibility applied last (outermost). Also: - AuthMiddleware now uses get_* instead of _get_* for proper component auth - Remove redundant _is_component_enabled checks (visibility is a transform) - Delete dead code (get_component method) - Add versions field to FastMCPMeta - Replace asserts with NotFoundError in component_service
marvin-context-protocol
Bot
added
enhancement
jlowin
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ade5a4838
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def __init__(self) -> None: | ||
| # Visibility is the first (innermost) transform - closest to the base provider | ||
| self._visibility = Visibility() | ||
| self._transforms: list[Transform] = [self._visibility] | ||
| self._transforms: list[Transform] = [] | ||
|
|
There was a problem hiding this comment.
Now that _transforms no longer includes Visibility, any code that still iterates self._transforms directly will skip visibility filtering. FastMCPProvider.get_tasks() still wraps only self._transforms, so provider-level enable/disable (or allowlist) won’t be applied to task registration for mounted servers, and disabled components can still be registered with Docket. This regression is introduced by separating visibility from _transforms; update FastMCPProvider.get_tasks() to iterate self.transforms instead (see src/fastmcp/server/providers/fastmcp_provider.py).
Useful? React with 👍 / 👎.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR refactors the server architecture to consolidate provider aggregation, revise error handling patterns, and introduce version-based component lookups. Key changes include: FastMCP now inherits from Provider with centralized list/get operations that aggregate results from sub-providers; visibility transforms are reordered to apply as the outermost layer; NotFoundError imports are replaced with explicit None checks and AuthorizationError raises; component lookups use VersionSpec for versioning; FastMCPMeta includes a new versions field; and middleware handling is unified through aggregated paths across multiple provider implementations. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/fastmcp/server/providers/__init__.py (1)
30-48: Confirm AggregateProvider belongs in top-level exports.If it’s a specialized utility, consider keeping it in the submodule to limit the public surface; otherwise this is fine. As per coding guidelines, keep init.py re-exports minimal.
src/fastmcp/server/server.py (1)
1104-1107: Silence unusedcontextargs in middleware call_next lambdas.
Rename the unused parameter (e.g.,_context) to satisfy lint and make intent explicit; apply similarly to the other call_next lambdas.♻️ Example tweak
- return await self._run_middleware( - context=mw_context, - call_next=lambda context: self.get_tools(run_middleware=False), - ) + return await self._run_middleware( + context=mw_context, + call_next=lambda _context: self.get_tools(run_middleware=False), + )Also applies to: 1222-1225, 1344-1349, 1466-1469, 2005-2008, 2031-2034
- get_*() now does aggregation + component auth (raises AuthorizationError) - Deleted _get_*() overrides - inherited from Provider applies transforms - Simplified AuthMiddleware to global auth only - Changed version params to VersionSpec | None (not str | None) - Updated tests to use _get_*() where visibility filtering is expected
Test Failure AnalysisSummary: Multiple versioning tests are failing because FastMCP-level transforms (like Root Cause: The PR refactors The transform chain flow:
The issue is that
Failing tests (all expect
Suggested Solution: The issue is subtle: Fix approach: Two options:
Looking at the code flow, I believe Option 2 is correct: when Detailed AnalysisError from logs: Test code change in PR (lines 486-492): # Can request specific versions in range
tool_v2 = await mcp.get_tool("add", VersionSpec(eq="2.0"))
assert tool_v2 is not None
assert tool_v2.version == "2.0"
# Cannot request version outside range - returns None
assert await mcp.get_tool("add", VersionSpec(eq="1.0")) is NoneThe test adds Current FastMCP.get_tool() flow:
Related Files
This is an automated analysis from the FastMCP test failure triage bot. Edit 1: Clarified root cause and transform chain flow. |
FastMCPProvider now calls _get_* methods instead of get_* to ensure nested server transforms are applied during lookups. Also converts string versions to VersionSpec in MCP handlers.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fastmcp/server/server.py (1)
906-962: Filter task‑eligible components before returning fromget_tasks.
get_tasks()currently returns all transformed components. When a nestedFastMCPis mounted, this can register non‑task components with Docket even if theirtask_configforbids tasks. The base Provider implementation filters bysupports_tasks(), so aligning here prevents accidental registration.🛠️ Proposed fix
- return [ - *transformed_tools, - *transformed_resources, - *transformed_templates, - *transformed_prompts, - ] + return [ + c + for c in [ + *transformed_tools, + *transformed_resources, + *transformed_templates, + *transformed_prompts, + ] + if c.task_config.supports_tasks() + ]
- Filter task-eligible components in FastMCPProvider.get_tasks() - Catch AuthorizationError in get_* methods and return None for consistency - Remove AggregateProvider from top-level exports
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/fastmcp/server/server.py (1)
1094-1126: Variable shadowing:ctxreused for different types.The variable
ctxfrom the async context manager (line 1094) is shadowed byctx = AuthContext(...)(line 1118). While the outer context isn't used after this point in this code path, this shadowing pattern is repeated acrossget_resources,get_resource_templates, andget_prompts, making the code harder to follow and potentially error-prone during future modifications.♻️ Suggested refactor to avoid shadowing
async with fastmcp.server.context.Context(fastmcp=self) as ctx: if run_middleware: mw_context = MiddlewareContext( message=mcp.types.ListToolsRequest(method="tools/list"), source="client", type="request", method="tools/list", fastmcp_context=ctx, ) return await self._run_middleware( context=mw_context, call_next=lambda context: self.get_tools(run_middleware=False), ) # Query through full transform chain (provider transforms + server transforms + visibility) tools = await self._list_tools() # Get auth context (skip_auth=True for STDIO which has no auth concept) skip_auth, token = _get_auth_context() # Filter by auth authorized: list[Tool] = [] for tool in tools: if not skip_auth and tool.auth is not None: - ctx = AuthContext(token=token, component=tool) + auth_ctx = AuthContext(token=token, component=tool) try: - if not run_auth_checks(tool.auth, ctx): + if not run_auth_checks(tool.auth, auth_ctx): continue except AuthorizationError: continue authorized.append(tool)Apply similar changes to
get_resources(line 1216),get_resource_templates(line 1317), andget_prompts(line 1414).
| async with fastmcp.server.context.Context(fastmcp=self) as ctx: | ||
| if run_middleware: | ||
| mw_context = MiddlewareContext( | ||
| message={}, # List resources doesn't have parameters | ||
| message={}, | ||
| source="client", | ||
| type="request", | ||
| method="resources/list", | ||
| fastmcp_context=fastmcp_ctx, | ||
| fastmcp_context=ctx, | ||
| ) | ||
| return list( | ||
| await self._run_middleware( | ||
| context=mw_context, | ||
| call_next=lambda context: self.get_resources( | ||
| run_middleware=False | ||
| ), | ||
| ) | ||
| return await self._run_middleware( | ||
| context=mw_context, | ||
| call_next=lambda context: self.get_resources(run_middleware=False), | ||
| ) |
There was a problem hiding this comment.
Inconsistent message types in MiddlewareContext.
The get_resources, get_resource_templates, and get_prompts methods use message={} (lines 1195, 1294, 1393) while get_tools uses a proper mcp.types.ListToolsRequest (line 1097). If middleware inspects the message type or contents, this inconsistency could lead to unexpected behavior.
🔧 Suggested fix for consistency
Consider using the appropriate MCP request types:
mw_context = MiddlewareContext(
- message={},
+ message=mcp.types.ListResourcesRequest(method="resources/list"),
source="client",
type="request",
method="resources/list",
fastmcp_context=ctx,
)Apply similar changes:
get_resource_templates: Use appropriate request type forresources/templates/listget_prompts: Usemcp.types.ListPromptsRequest(method="prompts/list")
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async with fastmcp.server.context.Context(fastmcp=self) as ctx: | |
| if run_middleware: | |
| mw_context = MiddlewareContext( | |
| message={}, # List resources doesn't have parameters | |
| message={}, | |
| source="client", | |
| type="request", | |
| method="resources/list", | |
| fastmcp_context=fastmcp_ctx, | |
| fastmcp_context=ctx, | |
| ) | |
| return list( | |
| await self._run_middleware( | |
| context=mw_context, | |
| call_next=lambda context: self.get_resources( | |
| run_middleware=False | |
| ), | |
| ) | |
| return await self._run_middleware( | |
| context=mw_context, | |
| call_next=lambda context: self.get_resources(run_middleware=False), | |
| ) | |
| async with fastmcp.server.context.Context(fastmcp=self) as ctx: | |
| if run_middleware: | |
| mw_context = MiddlewareContext( | |
| message=mcp.types.ListResourcesRequest(method="resources/list"), | |
| source="client", | |
| type="request", | |
| method="resources/list", | |
| fastmcp_context=ctx, | |
| ) | |
| return await self._run_middleware( | |
| context=mw_context, | |
| call_next=lambda context: self.get_resources(run_middleware=False), | |
| ) |
🧰 Tools
🪛 Ruff (0.14.13)
1203-1203: Unused lambda argument: context
(ARG005)
1 participant
FastMCP now inherits from Provider, establishing a cleaner class hierarchy. Previously these were parallel implementations; now FastMCP is-a Provider with aggregation capabilities on top.
What changed:
Provider provides the transform chain machinery - the
_get_*and_list_*methods that wrap raw lookups with transforms. FastMCP overrides_get_*to aggregate from multiple sub-providers before applying its own transforms. This isn't pure inheritance (aggregation requires custom logic), but it shares the transform chain pattern.Visibility is now consistently the last transform via a
.transformsproperty:Applied last means visibility sees components after transforms modify them - so tags added by transforms work with enable/disable filtering.
Other fixes:
get_*(with component auth) instead of_get_*(without)get_component, redundant_is_component_enabledchecks)NotFoundErrorin component_service